Fix broken TinyliciousClient test and ensure tinylicious tests run in CI #25344
Conversation
github-actions
bot
added
the
area: framework
There was a problem hiding this comment.
Pull Request Overview
This PR fixes a broken TinyliciousClient test and ensures tinylicious tests run in CI by adding the required :report script suffix. The test was failing due to incorrect API usage and promise handling when testing container disposal with data corruption errors.
- Fixed the broken TinyliciousClient test by correcting the runtime message submission API and simplifying error handling
- Added
test:realsvc:tinylicious:reportscripts to enable CI execution for packages with tinylicious tests - Removed unnecessary eslint disable comment
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/service-clients/tinylicious-client/src/test/TinyliciousClient.spec.ts | Fixed test implementation for container disposal with data corruption errors |
| packages/service-clients/tinylicious-client/package.json | Added :report script for CI compatibility |
| packages/service-clients/azure-client/package.json | Added :report script for CI compatibility |
| packages/framework/client-logger/fluid-telemetry/package.json | Added :report script for CI compatibility |
| (runtime as any).submit(ContainerMessageType.Alias, { id: alias }, resolve); | ||
| }).catch((error) => new Error(error.message)); | ||
| const corruptedAliasOp = (runtime: IContainerRuntime, alias: string): void => { | ||
| (runtime as any).submit({ type: ContainerMessageType.Alias, contents: { id: alias } }); |
There was a problem hiding this comment.
The submit method call appears to be using an incorrect API signature. The original code used separate parameters for message type and contents, but this change combines them into a single object. Verify this matches the expected IContainerRuntime.submit API signature.
| (runtime as any).submit({ type: ContainerMessageType.Alias, contents: { id: alias } }); | |
| (runtime as any).submit(ContainerMessageType.Alias, { id: alias }); |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
This is one of the ways in which the test was broken, which changed in #16276
| ): Promise<boolean | Error> => | ||
| new Promise<boolean>((resolve, reject) => { | ||
| runtime.once("dispose", () => reject(new Error("Runtime disposed"))); | ||
| (runtime as any).submit(ContainerMessageType.Alias, { id: alias }, resolve); |
There was a problem hiding this comment.
processAliasMessages internally invokes the localOpMetadata to resolve its own promise, which I assume is the only way this would have worked in the past? Though I don't see how it could have gotten to that point before erroring out as a malformed op. In any case, I don't think the test needs to await that resolution to remain valid.
| "test:realsvc:tinylicious": "start-server-and-test start:tinylicious:test 7070 test:realsvc:local:run", | ||
| "test:realsvc:tinylicious:report": "npm run test:realsvc:tinylicious", | ||
| "test:realsvc:tinylicious": "start-server-and-test start:tinylicious:test 7070 test:realsvc:tinylicious:run", | ||
| "test:realsvc:tinylicious:run": "mocha --recursive \"lib/test/**/*.spec.*js\" --exit", |
There was a problem hiding this comment.
Without --exit, the tests hang at completion. This implies something isn't getting cleaned up so it's not great, but this at least gets the tests running again.
Test was broken in a few distinct ways, but ultimately what it's trying to test is if a container gets disposed with an error, that error is propagated up to the IFluidContainer's matching event. This PR gets the test working again.
Additionally, the test wasn't running in CI because the CI scripts expect a
test:realsvc:tinylicious:reportscript (notice the:reportin particular). This change adds that script, which allows the test to run in CI. Also adds the script for all other packages that had atest:realsvc:tinyliciousscript.